Add enhanced presign request feature #1477
Conversation
There was a problem hiding this comment.
My review is incomplete, but I feel like the interface needs to be reworked to support correct serialization of structured header values and structured querystring parameters. I'm undecided what S3 client operations need to be supported. We need to better define the expected interface and features.
| # You should only set this value for testing. | ||
| def initialize(method, params = {}) | ||
| @bucket, @key = bucket_and_key(params) | ||
| @client = params.delete(:client) || Aws::S3::Client.new |
There was a problem hiding this comment.
We do this in multiple places, so this pattern is not new (inject or construct a client), but we do not actually require a client. We actually need a region, and a set of credentials. The client object in inconsequential. It seems like a better solution would be to have a public interface for getting the default region, and a public interface for getting default credentials.
The client configuration can be updated to use these interfaces, so the behavior remains consistent, and then the code can be reduced down to injecting an optional region and or credentials.
| def initialize(method, params = {}) | ||
| @bucket, @key = bucket_and_key(params) | ||
| @client = params.delete(:client) || Aws::S3::Client.new | ||
| @http_method = method[/[^_]+/].upcase |
There was a problem hiding this comment.
We can determine this correctly for every API operation by simply using the API object.
Aws::S3::Client.api.operation(method).http_methodThis has the benefit of it will work for more than object operations.
| @expires = expires_in(params) | ||
| @virtual_host = !!params.delete(:virtual_host) | ||
| @secure = params.delete(:secure) | ||
| @headers = build_headers(params) |
There was a problem hiding this comment.
This implementation of build_headers is limited to only support those params that have x-amz-* prefixes. As such you will run into issues with things like x-amz-metadata-* headers that are given as hashes normally, but this method will require them to be given as strings with metadata-{key} prefixes.
It would be ideal to preserve the same interface we use when making client calls.
|
Still in progress, yet have finished refractor using client request handler in constructing urls. |
|
Based on the discussion our team had off-line, we will work on a more generalized solution that brings much more benefits and flexibility in the long run (involving client level, request level etc.). And that is a feature has been prioritized in our backlog, and will be shipped through Modularization Ruby SDK which is currently under preview release and will be in GA in the near future. So this PR will not be merged. Meanwhile, This PR will still be kept opened for a while for feedbacks and thoughts! : ) |
awood45
force-pushed
the
master
branch
2 times, most recently
from
cc532a5
to
6e56849
Compare
|
@cjyclaire I am using v3 and the code below, but wondering how can I add a custom header to my PUT request. Adding it on the client-side throws an error, so it seems like I must do it while creating the pre-signed url? signer = Aws::S3::Presigner.new(client: s3)
self.presigned_url = signer.presigned_url(
:put_object,
bucket: self.bucket,
key: self.file_key
)So, is it possible right now in v3? If not, what would be the equivalent code for the code above with the gem mentioned above? Thank you! |
|
@oyeanuj Sure, you can do it both in V2 and V3, yet I'd recommend using the require `aws-sigv4`
# a s3 client at region 'us-west-2'
signer = Aws::Sigv4::Signer.new(
service: 's3',
region: 'us-west-2',
credentials_provider: client.config.credentials,
uri_escape_path: false,
)
# create presigned url for an object with bucket 'a-fancy-bucket' and key 'hello_world'
url = signer.presign_url(
http_method: 'PUT',
url:'https://a-fancy-bucket.s3-us-west-2.amazonaws.com/hello_world',
headers: {
"x-amz-foo" => "bar"
}
body_digest: 'UNSIGNED-PAYLOAD',
)
# making request
body = ...
Net::HTTP.start(url.host) do |http|
http.send_request("PUT", url.request_uri, body, {
"x-amz-foo" => "bar",
})
end
# => #<Net::HTTPOK 200 OK readbody=true> |
|
@cjyclaire Thanks! Just curious, is there an advantage of using |
cjyclaire
added
needs-major-version
|
Closing - I think this solution solves this case? #874 (comment) |
This PR adds support for
Aws::S3::PresignedRequestthat allows more flexible and customized presigned behaviors, refractored based onaws-sigv4gem and provides helpful#headersfor bucket policy work arounds etc..Documentations and tests(rspec & smoke) both have been addressed in the PR, here is a sample usage:
Related issues, PRs and feature requests:
#874, #1152, #1384
#1228, #1403 ( #1400, #1401)
#2098
cc:\ @awood45 @trevorrowe